Move ethkey crypto utils to parity crypto crate by grbIzl · Pull Request #210 · paritytech/parity-common

grbIzl · 2019-09-02T11:49:57Z

The intention of this PR is to remove dependency to accounts\ethkey for parity components, that use crypto utils from it. That would allow further decomposition of parity repo. Including move of secret store into the separate repo.
Additional comments:

I straightforwardly merged code from ethkey into parity crypto, that is responsible for work with key pairs, encryption etc.

I left wallets code (brain, brain prefix etc) in ethkey. Also left password.rs there
I didn't do any renaming and reorganization of parity crypto crate. May be it's worth doing, but I don't have good ideas and would be grateful for any suggestions.

I'm not going to consider substrate code, related to the same crypto primitives. It's left for the further development.

ordian

I haven't looked deeply at the code as it seems to be copied, but have some concerns about dependencies.

dvdplm

One thing I'm not clear about is if the code added here is entierly ethereum-specific or if there is stuff that is useful to other projects as well. You mentioned secret-store might be extracted after this. Are there other Parity projects that use/could use the crypto here?
/cc @cheme

dvdplm · 2019-09-03T09:04:45Z

+	}
+
+	/// Inplace add one secret key to another (scalar + scalar)
+	pub fn add(&mut self, other: &Secret) -> Result<(), Error> {


There seems to be a lot of similar mathy methods here as well as in math.rs: maybe there's a trait here waiting to be discovered and impl'd for Secret and Public? add, sub, mul, neg, is_valid etc?

But it's different algebraic groups (scalar and elliptic curve). I'm afraid, that general trait for them could lead to mistreatments

Mistreatments, can you elaborate on that? How could it be abused?

ordian · 2019-09-03T09:18:59Z

Also see #80 for previous attempt to add secp256k1 to parity-crypto (cc @cheme).

grbIzl · 2019-09-03T10:09:53Z

Also see #80 for previous attempt to add secp256k1 to parity-crypto (cc @cheme).

IMHO @cheme was too ambitious in his PR :-) I want to achieve simpler objectives (move common code from ethkey and don't break anything dependent on parity-crypto). And we need to have it done, if we want to develop secret store further as a separate project. So I hope to bring it to the (happy) end.

grbIzl · 2019-09-03T10:18:25Z

One thing I'm not clear about is if the code added here is entierly ethereum-specific or if there is stuff that is useful to other projects as well. You mentioned secret-store might be extracted after this. Are there other Parity projects that use/could use the crypto here?

The primary beneficiary of this PR is secret store indeed. I'm not aware of other projects for sure. At the same time, we wanted to extract account management, removing this dependency from it could help.

grbIzl · 2019-09-03T10:19:51Z

@dvdplm thanks for comments! It will be good to cleanup this code, as it was not touched for a while.

dvdplm · 2019-09-03T10:58:20Z

@grbIzl I really appreciate the effort and the goal here! FWIW I tried something similar a while back and failed.
It's a substantial chunk of code added though so I want to collect opinions from more people on where we want this to live. I think it's good to get as many eyes on it as possible so we avoid bitrot.

dvdplm · 2019-09-03T14:28:06Z

Are there other Parity projects that use/could use the crypto here?

Substrate uses parity-crypto in substrate-keystore, @grbIzl, we need to make sure that when this PR is in good shape we submit upgrade PRs to both parity-ethereum and substrate.

grbIzl · 2019-09-16T14:07:36Z

I put newly added dependencies under the build feature (publickey).

The only not implemented major thing is including of ethereum_types crate. H256 is used heavily in Signature and Secret classes and I would need to copypaste its functionality there. So for now I leave it as a dependency (but put under the same feature)

ordian

Looks good in general, some minor suggestions.

cheme

With things moved inside a feature gated module, I guess we can have ethereum-type dependency and some former code moved with less hesitation.

cheme · 2019-09-16T15:59:53Z

+
+[features]
+default = []
+publickey = ["eth-secp256k1", "lazy_static", "ethereum-types"]


I am not sure about the rational of naming the feature 'publickey' , a comment could be good.

Does it make sense to name this feature secp256k1? publickey seems too general

I agree in general, but this exact name (secp256k1) seems not the best option, because we already have eth-secp256k1 feature and it will be two similar names. Naming is the hardest part of coding :-(

dvdplm

Good stuff here. I got ~half way through. Mostly nits and (I believe) mostly on old code that was copied over.

dvdplm · 2019-09-16T18:37:00Z

I think it's unfortunate that git history has not been preserved for these files as they migrated from the parity-ethereum repo. If it's too complex to do then we should at least make a note of where to look for it back in ethkey.

grbIzl · 2019-09-18T15:35:59Z

I think it's unfortunate that git history has not been preserved for these files as they migrated from the parity-ethereum repo. If it's too complex to do then we should at least make a note of where to look for it back in ethkey.

It's a common problem for such case, when files are moved to the other repo. So I've put a comment, but not sure, how helpful it will be

dvdplm

just a few minor nits

dvdplm · 2019-09-23T07:39:21Z

+		let secret = Secret::from_str("a100df7a048e50ed308ea696dc600215098141cb391e9527329df289f9383f65").unwrap();
+		let mut public = generation_point();
+		public_mul_secret(&mut public, &secret).unwrap();
+		assert_eq!(format!("{:x}", public), "8ce0db0b0359ffc5866ba61903cc2518c3675ef2cf380a7e54bde7ea20e6fa1ab45b7617346cd11b7610001ee6ae5b0155c41cad9527cbcdff44ec67848943a4");


If this value is from a spec somewhere it'd be great to document that. :)

It's just a randomly created secret

dvdplm · 2019-09-23T07:40:03Z

 name = "bench"
 harness = false
-
+required-features = ["publickey"]


dvdplm · 2019-09-23T07:41:23Z

It's a common problem for such case, when files are moved to the other repo.

It is. The trick is to using something like git subtree extract. I should have spoken up when you started this work. :/

dvdplm

a few minor things

dvdplm · 2019-09-23T07:43:57Z

 [package]
 name = "parity-crypto"
-version = "0.4.1"
+version = "0.4.2"


Just a thought: while it's technically correct that the changes here are not breaking the previous API, I wonder if it's not warranted to go with 0.5 here anyway, just to signal the presence of a major new piece of functionality?
/cc @ordian

I see where you're coming from and don't have a strong opinion on this. Having a minor release makes it easier to ensure other crates could use the latest version of parity-crypto (even via transitive dependencies), but that's a minor concern.

dvdplm · 2019-09-23T07:49:59Z

+	}
+
+	/// Compute power of secret key inplace (secret ^ pow).
+	/// This function is not intended to be used with large powers.


Why not? And what is the limit? And what happens if one does? Panic? Panic or overflowing behaviour should be documented.

I've removed this comment as it's misleading. The only limitation here is pow (usize). The multiplication on elliptic curves doesn't have any limitations

dvdplm

🎉

So what are the next steps after this? A PR to parity-ethereum to use this I guess?

grbIzl · 2019-09-24T08:53:05Z

So what are the next steps after this? A PR to parity-ethereum to use this I guess?

Yep.

dvdplm · 2019-09-24T09:32:28Z

k, I'm putting this onice so it doesn't get merged before it's time: @grbIzl will prepare a PR to parity-ethereum to make double extra sure nothing breaks and then we can merge this.

ordian · 2019-09-24T09:33:17Z


 [dependencies]
 tiny-keccak = "1.4"
+eth-secp256k1 = { git = "https://github.com/paritytech/rust-secp256k1", rev = "a96ad75", optional = true }


Suggested change

eth-secp256k1 = { git = "https://github.com/paritytech/rust-secp256k1", rev = "a96ad75", optional = true }

eth-secp256k1 = { git = "https://github.com/paritytech/rust-secp256k1", rev = "72c93ab = true }

another update

niklasad1 · 2019-10-22T08:22:58Z

@grbIzl @dvdplm let's publish a new version of parity-crypto so it can be used by openethereum/parity-ethereum#11174?

grbIzl · 2019-10-22T11:00:11Z

@grbIzl @dvdplm let's publish a new version of parity-crypto so it can be used by paritytech/parity-ethereum#11174?

Working on it. Right now bumped into paritytech/rust-secp256k1#25

grbIzl added 3 commits September 2, 2019 12:10

EthKey modules moved to crypto crate

fd0e2ec

2018 edition changes

fc1298f

2 converters added

f1cf5dc

This comment has been minimized.

Sign in to view

grbIzl added the M4-core label Sep 2, 2019

ordian reviewed Sep 2, 2019

View reviewed changes

Comment thread parity-crypto/Cargo.toml Outdated

Comment thread parity-crypto/Cargo.toml Outdated

Comment thread parity-crypto/Cargo.toml Outdated

Comment thread parity-crypto/Cargo.toml

Remove quick error

a6f8c9f

grbIzl added the A4-gotissues label Sep 2, 2019

dvdplm suggested changes Sep 3, 2019

View reviewed changes

grbIzl added 5 commits September 10, 2019 13:23

Public key primitives moved to the separate module

6940745

Feature include for public key module

3c047ed

Rust hex version increased, module docs added

cad7824

Move ethereum types include under public key feature

f6c4702

More specific names for files

9b7a0c9

grbIzl added A0-pleasereview and removed A4-gotissues labels Sep 16, 2019

grbIzl requested review from cheme, dvdplm and ordian September 16, 2019 14:07

ordian reviewed Sep 16, 2019

View reviewed changes

Comment thread parity-crypto/Cargo.toml Outdated

Comment thread parity-crypto/Cargo.toml Outdated

Comment thread parity-crypto/src/publickey/error.rs Outdated

cheme reviewed Sep 16, 2019

View reviewed changes

dvdplm suggested changes Sep 16, 2019

View reviewed changes

grbIzl added 5 commits September 17, 2019 12:19

Errors usages corrected

34649b5

Documentation for module components improved

c1ba58a

Test and benchmark added

377db06

Static initialization for curve order method

3a24be0

Couple of comments changed

dc7dc4d

grbIzl added 3 commits September 19, 2019 10:14

Generation point bytes glued into one array

46ece97

Tests for math operations added

7ca69cc

Proper feauture for benches

5a77f83

dvdplm reviewed Sep 23, 2019

View reviewed changes

dvdplm suggested changes Sep 23, 2019

View reviewed changes

grbIzl added 3 commits September 23, 2019 11:32

Comments added to methods and parameters added

a69d130

Switch to try_from and Infallible traits

2acb917

Comment removed

fedfc96

dvdplm approved these changes Sep 23, 2019

View reviewed changes

ordian approved these changes Sep 24, 2019

View reviewed changes

dvdplm added A1-onice and removed A0-pleasereview labels Sep 24, 2019

ordian reviewed Sep 24, 2019

View reviewed changes

Ethereum types version increased

669933f

grbIzl merged commit 0de0098 into master Oct 22, 2019

grbIzl deleted the EthKeyCrypto branch October 22, 2019 07:51

	eth-secp256k1 = { git = "https://github.com/paritytech/rust-secp256k1", rev = "a96ad75", optional = true }
	eth-secp256k1 = { git = "https://github.com/paritytech/rust-secp256k1", rev = "72c93ab = true }

Conversation

grbIzl commented Sep 2, 2019

Uh oh!

This comment has been minimized.

ordian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ordian commented Sep 3, 2019

Uh oh!

grbIzl commented Sep 3, 2019

Uh oh!

grbIzl commented Sep 3, 2019

Uh oh!

grbIzl commented Sep 3, 2019

Uh oh!

dvdplm commented Sep 3, 2019

Uh oh!

dvdplm commented Sep 3, 2019

Uh oh!

grbIzl commented Sep 16, 2019

Uh oh!

ordian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cheme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dvdplm commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grbIzl commented Sep 18, 2019

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

dvdplm commented Sep 16, 2019 •

edited

Loading

dvdplm commented Sep 24, 2019 •

edited

Loading